Repository navigation
Conversation
…ename On macOS, fseventsd can occasionally replay directory/file creation events after the native watcher starts, racing with assertions that expect no changes to have been reported yet. Add a short delay before starting the watcher to reduce (not eliminate) the chance of this, and treat the known race as a skip rather than a failure if it still occurs for one of the newly created entries. Fixes dotnet#135283 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2aac025-5dc1-420e-bfdf-19dd4b2f2f6f
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Contributor
|
Tagging subscribers to this area: @dotnet/area-extensions-filesystem |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
SkipTestException requires [ConditionalFact]; under [Fact], the race still fails the test.
1 open finding
What changed in this PR
Mitigates a macOS FSEvents race affecting a PhysicalFileProvider rename test.
Changes:
- Adds a macOS-only delay before watcher registration.
- Dynamically skips when pre-rename tokens already fired.
| File | Description |
|---|---|
PhysicalFileProviderTests.cs |
Adds macOS race mitigation and skip handling. |
🧠 Review effort: Balanced
This was referenced Oct 7, 2026
Closed
[Fact] uses the plain xunit test case, which has no special handling for SkipTestException, so throwing it there was reported as an ordinary failure instead of mitigating the macOS flake. [ConditionalFact] (even with no condition arguments) wraps the test case with Microsoft.DotNet.XUnitExtensions' SkippedTestMessageBus, which recognizes SkipTestException and converts it to a skip. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2aac025-5dc1-420e-bfdf-19dd4b2f2f6f
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The timing workaround introduces an untracked quarantine where deterministic native-event filtering can preserve test coverage.
1 open finding
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Per review feedback: this test only needs the injected rename (fileSystemWatcher.CallOnRenamed), which calls FileSystemWatcher.OnRenamed directly and bypasses Filter matching (MatchPattern is only applied in the NotifyRenameEventArgs/NotifyFileSystemEventArgs methods that wrap genuine native events). Setting Filter to a pattern that cannot match the GUID-generated test entries suppresses any genuine native events - including delayed macOS fseventsd replays of the pre-watch directory/file creation - while leaving the explicitly injected rename event unaffected. This removes the need for the artificial delay and the dynamic skip, and keeps the pre-rename assertions deterministic on all platforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2aac025-5dc1-420e-bfdf-19dd4b2f2f6f
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The filter correctly suppresses native events while CallOnRenamed bypasses filtering as intended.
0 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Mitigates the flaky
Microsoft.Extensions.FileProviders.PhysicalFileProviderTests.TokensFiredForNewDirectoryContentsOnRenametest on macOS.Root cause
The test creates a real directory/subdirectory/file structure on disk, then starts a real
FileSystemWatcher-backedPhysicalFilesWatcher/PhysicalFileProviderwatch, and asserts the corresponding change tokens have not fired yet (before the test's simulated rename). On macOS, the native watcher is backed byfseventsd, a separate daemon that the process subscribes to asynchronously. As documented by a .NET team member investigating a related issue (#30415 (comment)):So even though the watcher requests
kFSEventStreamEventIdSinceNow, there is no documented guarantee it excludes events for changes made shortly before the stream starts. This occasionally causes the "new directory/subdirectory/file should not have changed yet" assertions to fail.This is architecturally specific to macOS: Windows (
ReadDirectoryChangesW) and Linux (inotify) are handle/descriptor-based with no separate daemon replaying historical events.Why not fix it in
FileSystemWatcher/PhysicalFilesWatcherinsteadkFSEventStreamEventIdSinceNow,0.0flatency,NoDefer), so there isn't more headroom at that layer.PhysicalFilesWatcher-side fix (e.g. snapshotting expected state at watch-registration time and suppressing events that predate it) could close this deterministically, but it's a meaningfully more invasive change for a bug class that, in production, results in at most a spurious/redundant change notification — not a missed one or a correctness issue. That cost/benefit doesn't justify it here, so this PR only addresses the test.Fix
The test only needs one injected event:
fileSystemWatcher.CallOnRenamed(...), which invokesFileSystemWatcher.OnRenameddirectly. That method does not performFiltermatching itself — pattern matching againstFilteronly happens inNotifyFileSystemEventArgs/NotifyRenameEventArgs, which wrap genuine native OS events before invoking the protectedOnChanged/OnCreated/OnRenamedmethods (seesrc/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.cs).So the test now sets
fileSystemWatcher.Filterto a pattern that cannot match any of its (randomly generated, GUID-named) test entries before starting the watch. This deterministically suppresses any genuine native events — including delayedfseventsdreplays of the pre-watch directory/file creation — while leaving the explicitly injected rename event unaffected, since it bypassesFilterentirely. This removes the need for an artificial delay or a dynamic skip, and keeps all the pre-rename assertions active and deterministic on every platform.Fixes #135283
Note
This PR description and change were drafted with AI (GitHub Copilot) assistance.